Hi,

thanks for this- it looks great. One thing I think we should consider/discuss is whether we could add the preemption retry fields as first-class proto fields rather than relying on annotations. Reasoning here is that anottions are very easy to add at first as they require no interface changes, but can soone get quite hard to work with as evwerything is just a map[string]string.

Personally I'd be in favour of adding these fields to schedulerobjects.JobSchedulingInfo right now with the view of adding them to api.Job once the feature is stable.

One comment I

Personally I'd be in favour of adding these fields to schedulerobjects.JobSchedulingInfo right now with the view of adding them to api.Job once the feature is stable.

I looked into this but it seems a bit difficult to keep it is an annotation and not a first class citizen.

I'm not able to create the scheduling info from an armadaevents.SubmitJob since the annotations aren't there. I think there would also need to be some more thinking about how the global preemption retry config integrates into this. Might need to move that into the scheduler ingester to more elegantly handle that unless we want to reference the global preemption retry config all the time.

@jparraga-stackav Thanks for the work on this.
I was wondering if we could also handle imminent node shutdown scenarios. In addition to the current retry logic for pod evictions, it might be useful to check for pods terminated with the reason:

Pod was terminated in response to imminent node shutdown.

We could incorporate a simple check on pod.Status.Message to capture these cases. This would help cover both graceful shutdowns and node preemptions for various reasons.

@jparraga-stackav Thanks for the work on this. I was wondering if we could also handle imminent node shutdown scenarios. In addition to the current retry logic for pod evictions, it might be useful to check for pods terminated with the reason:

Pod was terminated in response to imminent node shutdown.

We could incorporate a simple check on pod.Status.Message to capture these cases. This would help cover both graceful shutdowns and node preemptions for various reasons.

I will likely look into this as a follow up improvement.